Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Filebeat subcommand: generate #9314

Merged
merged 5 commits into from
Feb 8, 2019

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Nov 30, 2018

This PR exposes the existing Filebeat module generators as generate subcommand.

$ ./filebeat generate -h
Generate Filebeat modules, filesets and fields.yml

Usage:
  filebeat generate [command]

Available Commands:
  fields      Generates a new fields.yml file for fileset
  fileset     Generates a new fileset
  module      Generates a new module

The subcommands module, fileset and fields use the same code as the scripts.

I added E2E tests to validate the functionality.

Blocked by #9147 as it contains the refactorings of that PR.

@kvch kvch added review Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. blocked needs_docs labels Nov 30, 2018
@kvch kvch requested a review from ph November 30, 2018 16:24
@kvch kvch force-pushed the feature-filebeat-expose-generator-cmd branch from 016f4c2 to a220213 Compare December 3, 2018 08:30
}

genModuleCmd.Flags().String("modules-path", defaultHomePath, "Path to modules directory")
genModuleCmd.Flags().String("es-beats", defaultHomePath, "Path to Elastic Beats")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to expose both, because by default both of the flags will point to the same place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Not yet. Btu when community Beats will support a similar module-handling, we will need this.

@kvch
Copy link
Contributor Author

kvch commented Dec 3, 2018

jenkins test this

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, WFG

@kvch kvch removed the blocked label Dec 3, 2018
@kvch kvch force-pushed the feature-filebeat-expose-generator-cmd branch from 40f7599 to e7901be Compare January 8, 2019 10:35
@kvch kvch requested a review from a team as a code owner January 8, 2019 10:35
@kvch
Copy link
Contributor Author

kvch commented Jan 8, 2019

Rebased

@kvch kvch removed the request for review from a team January 8, 2019 10:46
@kvch kvch force-pushed the feature-filebeat-expose-generator-cmd branch from fa4afbc to ea07b1a Compare January 9, 2019 09:07
@ruflin
Copy link
Member

ruflin commented Jan 9, 2019

If we expose this to the user, I think we should also add some docs on how we expect the users to use this. How will the user handle the new fields?

The PR has needs_backport label. Is there a reason we need this in 6.x?

@kvch
Copy link
Contributor Author

kvch commented Jan 9, 2019

👍 on the docs, that's why the PR has the needs_docs label. What do you mean first question?

Obviously, we don't "need" it, but we planned to release it in 6.x. Why do you think it is problem?

@ruflin
Copy link
Member

ruflin commented Jan 9, 2019

A user creates a module with this command, how will he load / configure the additional fields that are used for it? If the user upgrades to a newer version, will the module stay intact? What if he wants the module on multiple machines?

@kvch kvch removed the needs_backport PR is waiting to be backported to other branches. label Jan 15, 2019
@kvch
Copy link
Contributor Author

kvch commented Jan 16, 2019

I am still not sure if I follow you. The behaviour of the generators has not changed. In this PR I only added new commands and subcommands to Filebeat to make existing functionality accessible to more people.

After a user generates a module and a fileset, he/she can add the fields of the fileset to the appropriate fields.yml file. generate fields is not able to produce a fields.yml which is 100% guaranteed to match the actual fields. It is just a "crutch" for a user to get started with the file. The generator explicitly asks the user to check the result. It also generates fields with the description which need to be filled in.

Why wouldn't the module stay intact during an upgrade? If the name of the user's module is the same as a new module coming with the new release, it gets overwritten. But this has been the behaviour since introducing module generators in Filebeat.

If he/she wants to have this module on multiple hosts, it is possible to copy them as before. Nothing changes here, also.

@kvch kvch mentioned this pull request Jan 16, 2019
2 tasks
@andrewkroh
Copy link
Member

Nice addition to Filebeat. An accompanying filebeat test module command that uses the _ingest/pipeline/_simulate API to test the pipeline and validate the resulting document would also help make module development more accessible.

@ruflin
Copy link
Member

ruflin commented Jan 17, 2019

So far this functionality was only accessible to devs in the Beats repo and was not really intended for production but more for module development. With the above we make it accessible to all users and production use cases.

One question I don't fully understand yet is what happens with the fields.yml. If the above commands are used in a dev environment and make update is run, they are put into the global fields.yml file and fields.go inside the binary. If someone uses the above command outside the dev environment, he gets the fields.yml in the module (which we don't ship) but AFAIK it's not really usable and will not have any effect on the template.

To be clear: I really like to make the above commands available to all our users but I want to make sure they have the experience they expect when they run the commands.

@kvch
Copy link
Contributor Author

kvch commented Jan 18, 2019

Some parts of make update are not yet ported to Golang. So it is not yet possible to generate everything for a module.
@ruflin I am OK with putting this PR on hold until all generators can be exposed. WDYT?

@ruflin
Copy link
Member

ruflin commented Jan 18, 2019

@kvch Even if mage update would be all in Go, there would be still an issue here. The reason is that by default even if there is a fields.yml, the fields from the binary will taken. Even if a user adds his fields to the global fields.yml, it will not read by default.

To keep this moving forward few ideas:

  1. We recommend users to copy their fields to append_fields
  2. We tell users that fields.yml does not work yet
  3. We introduce some logic in the module that can read the local fields.yml in the module and append it to the template if it is there.

To get this PR in quickly, we could start with 2 and see what the user feedback will be and work on 3. I personally like 3 because it will make modules "just work".

@kvch
Copy link
Contributor Author

kvch commented Jan 18, 2019

@ruflin Indeed. I like what you have proposed in option 3. I will open a new PR with the changes which lets users read their local fields.yml file. That is a nice addition on its own.

@kvch
Copy link
Contributor Author

kvch commented Jan 18, 2019

I only need to investigate why the test cannot find the test pipeline on Windows.... >.<

@kvch
Copy link
Contributor Author

kvch commented Feb 1, 2019

jenkins test this

@kvch kvch force-pushed the feature-filebeat-expose-generator-cmd branch from d4baa47 to 649e95a Compare February 4, 2019 08:41
@kvch kvch force-pushed the feature-filebeat-expose-generator-cmd branch from 649e95a to a30035d Compare February 4, 2019 14:55
@kvch
Copy link
Contributor Author

kvch commented Feb 4, 2019

jenkins test this

@kvch
Copy link
Contributor Author

kvch commented Feb 8, 2019

Failing tests are unrelated.

@kvch kvch merged commit cc9a326 into elastic:master Feb 8, 2019
@kvch kvch added the needs_backport PR is waiting to be backported to other branches. label Feb 8, 2019
@kvch
Copy link
Contributor Author

kvch commented Feb 8, 2019

Needs backport to 7.x branch...

@kvch kvch removed the needs_backport PR is waiting to be backported to other branches. label Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants